@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)

diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index 16c35bcf49339..d16f32bf618ee 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -299,10 +299,7 @@ class LangOptionsBase {
   };
 
   /// Possible float expression evaluation method choices.
-  enum FPEvalMethodKind {
-    /// The evaluation method cannot be determined or is inconsistent for this
-    /// target.
-    FEM_Indeterminable = -1,
+  enum FPEvalMethodKind : unsigned {
     /// Use the declared type for fp arithmetic.
     FEM_Source = 0,
     /// Use the type double for fp arithmetic.

@tstellar hi, I'm not sure the process for merging to a release branch, this is a minor correctness fix but it prevents a massive amount of warning spam when building with more current clangs. cc'ing @AaronBallman and @zahiraam so they see this and can weigh in on whether it is reasonable to pick this up.

hi, I'm not sure the process for merging to a release branch

https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

@ojhunt I don't think yo need to create a PR to backport. These are the steps.

1. Open a issue with the Title: Please backport <thehashyouwanttobackport>.
2. Add this comment: /cherry-pick <thehashyouwanttobackport>.
3. Assign the issue to the LLVM 20.x Release Milestone.

The backport should happen automatically.

@zahiraam @AaronBallman I'm not sure if this warrants the cherry pick though - because we cannot actually trigger the use of FEM_Indeterminable the fact that the behavior would be wrong isn't relevant, the problem is just the warning spam you get while building with trunk clang - which could be mitigated by people disabling the warning in their cmake config?

I don't see a significant need to backport the changes, which is a good thing because I don't think we can backport this: it's an ABI breaking change (changes the size of the enumeration due to gaining a fixed underlying type).

I agree, it's literally just a warning correction, and people building this release of llvm with a newer clang can configure it to disable this warning - I just wasn't sure what the general policy around this kind of thing was which is why I made a PR and asked for advice :D

@AaronBallman that said I'm not sure this change to the underlying type would impact anything - the unsigned and int are the same size, and the value is stored in an explicitly unsigned bitfield which is the source of the warning.

I agree, it's literally just a warning correction, and people building this release of llvm with a newer clang can configure it to disable this warning - I just wasn't sure what the general policy around this kind of thing was which is why I made a PR and asked for advice :D

@AaronBallman that said I'm not sure this change to the underlying type would impact anything - the unsigned and int are the same size, and the value is stored in an explicitly unsigned bitfield which is the source of the warning.

FWIW, I was thinking less about the size and more about the range of valid values for the enumeration.

I agree, it's literally just a warning correction, and people building this release of llvm with a newer clang can configure it to disable this warning - I just wasn't sure what the general policy around this kind of thing was which is why I made a PR and asked for advice :D
@AaronBallman that said I'm not sure this change to the underlying type would impact anything - the unsigned and int are the same size, and the value is stored in an explicitly unsigned bitfield which is the source of the warning.

FWIW, I was thinking less about the size and more about the range of valid values for the enumeration.

ah - I thought you were thinking in terms of the ms padding behavior :D